fix: add bounds assertions and safety docs for FlexBuffer OOB reads#9155
Open
samrigby64 wants to merge 1 commit into
Open
fix: add bounds assertions and safety docs for FlexBuffer OOB reads#9155samrigby64 wants to merge 1 commit into
samrigby64 wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Three locations in flexbuffers.h can trigger out-of-bounds reads (CWE-125) when called on unverified input: 1. Sized::read_size() (line ~241): reads `byte_width_` bytes before data_. A malformed byte_width_ (not 1/2/4/8) or a data_ pointer too close to the buffer start causes a read before the allocation. 2. Map::Keys() (line ~351): accesses data_ - byte_width_*3 for the three Map prefix fields. Same root cause as google#1. 3. Vector::operator[] (line ~962): uses size() — which calls read_size() — as the loop bound, then reads type bytes at data_ + len*byte_width_. A corrupt len can push this read arbitrarily past the buffer end. The FlexBuffer Verifier already catches all three patterns when flexbuffers::VerifyBuffer() is called before data access (it calls VerifyBeforePointer for the size prefix and VerifyFromPointer for the element and type-byte spans). These changes make the contract explicit: - Add FLATBUFFERS_ASSERT in read_size() and Keys() that byte_width_ is one of the four legal values; a corrupt value will now fire in debug builds. - Add clarifying comments at all three sites referencing the Verifier requirement and the CWE number so callers understand the precondition. Reported by: Sam Rigby (samrigby432@outlook.com)
samrigby64
force-pushed
the
fix/flexbuffer-oob-reads
branch
from
f1842d9 to
1164b20
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Three locations in
flexbuffers.hcan trigger out-of-bounds reads (CWE-125) when called on unverified input:Sized::read_size()(line ~241) readsbyte_width_bytes beforedata_. A malformedbyte_width_(not 1/2/4/8) or a data_ pointer too close to the buffer start causes a read before the allocation.Map::Keys()(line ~351) accessesdata_ - byte_width_*5for the three Map prefix fields. Same root cause as Don't just blindly double the buffer size #1.Vector::operator[](line ~962) usesread_size()— which callsread_size()— as the loop bound, then readstypebytes atdata_ + len*byte_width_. A corruptlencan push this read arbitrarily past the buffer end.The FlexBuffer Verifier already catches all three patterns when
flexbuffers::VerifyBuffer()is called before data access (it callsVerifyBeforePointerfor the size prefix andVerifyFromPointerfor the element and type-byte spans). These changes make the contract explicit.Changes
FLATBUFFERS_ASSERTinread_size()andKeys()thatbyte_width_is one of the four legal values; a corrupt value will now fire in debug builds.Testing
No behavioral change for verified buffers; asserts fire only on malformed input that the Verifier would already have rejected. This is a hardening / defense-in-depth change.